-
Notifications
You must be signed in to change notification settings - Fork 515
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
If every type of an expression is the same return that type as a result #4211
Conversation
3dfd4f8
to
d6f7c08
Compare
Do you have some public information what do you try to fix or change? |
Yeah, I noticed that for MySQL, However, I found a new example that doesn't work here,
(I'm still trying to simplify the repro case further) This errors:
The reason is the types are slightly different
We can instead check for distinct dialect type and this will work. |
Actually I think this is fully broken. We're ignoring the passed in types list, so we're just returning the input type as the output type in many cases when it's not legal. So for example
Calling |
Yes, we don't have a real function representation which validates (and infers) the parameters, we only added support for the return type. |
I think instead I will try to reimpl all the functions in Mysql type resolver to support mysql specific types. For example,
this will check all other integer types like BIG_INT. Maybe there is a way to do this more generically, like telling the ansi resolver that INTEGER is analogous to BIG_INT? But for now, I will try to unblock our upgrade to 2.0 by redefining the functions for mysql. |
What do you mean by upgrading, this did work in 1.5.5? |
We are on 1.5.4 and the examples I gave worked. I am upgrading to 2.0 and it does not work (same story on latest master). |
@hfhbd do you know if there is a reason we can't map BIG_INT to Ansi INTEGER? In ExprUtil.kt,
This used to handle multiplication for MySQL, but now it doesn't work since BIG_INT isn't in this list.
(I added some extra log) |
BigInt is 64 bit, mapped to kotlin.Long and Integer is 32 bit, mapped to kotlin.Int. |
I don't think so
INTEGER is Long |
This is true for PrimitiveType.INTEGER, but MySql has its own Integer type: MySqlType.INTEGER: sqldelight/dialects/mysql/src/main/kotlin/app/cash/sqldelight/dialects/mysql/MySqlType.kt Line 29 in 3a62ba0
|
But I'm asking if we can map MySQL.BIG_INT to PrimitiveType.INTEGER, I am not talking about MysqlType.INTEGER |
Do you still want to write your own MySql dialect? If so, I guess (!) you could simple remove |
I don't want to write my own, we already use the builtin one in this repo. I'm just wondering why this was changed for 2.0 since it's not obvious to me. Replacing it with PrimitiveType would be a workaround but we should probably support all mysql types for binary expressions like multiplication, I'll look at modifying the dialect |
No description provided.